Refactor RejectTransaction to CancelIOUTransaction #11720

mountiny · 2022-10-11T12:11:01Z

Details

As part of the API unchained, we are refactoring our API calls so they are optimistic. Here the action of cancelling/declining a money request is made optimistic by creating the report action locally and then passing the new id to our API.

Thanks to this, user can cancel money request offline and if everything was fine, this will be autocompleted once they get online.

In case of error, red brick road will lead him to the error showing him that cancellation failed.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/213615
$ #8722

Tests

This must be tested alongside with https://github.com/Expensify/Web-Expensify/pull/35088

1. OFFLINE

Go offline
Create a money request from User A to user B
Create another money request
As user A, go to the IOU details and cancel this IOU request
Make sure the request has been cancelled and the IOU preview has been updated (the message should be greyed out)
Go online and make sure that the cancel request is no longer greyed out
Refresh and switch chats there and back and make sure everything is fine
Create a money request from User A to user B
Create another money request
As user B, go offline
Go to the IOU details and decline this IOU request
Make sure the request has been declined and the IOU preview has been updated (the message should be greyed out)
Go online and make sure that the cancel request is no longer greyed out
Refresh and switch chats there and back and make sure everything is fine

2. ONLINE

Repeat offline steps, but skip any steps that ask you to go offline
Make sure that the requests are cancelled/declined

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Same as test steps.

Verify that no errors appear in the JS console

Screenshots

Web

web.mov

Mobile Web - Chrome

Mobile Web - Safari

safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

melvin-bot · 2022-10-11T12:11:06Z

Looks like you modified deprecatedAPI.js! To be clear, you should not be adding any code to this file.

Instead, all new API commands should use API.js, and follow our guidelines for writing new API commands.

Unsure if your change is okay? Drop a note in #expensify-open-source!

src/libs/actions/IOU.js

;

src/components/ReportTransaction.js

src/libs/actions/IOU.js

Santhosh-Sellavel · 2022-11-04T16:45:24Z

@mountiny or @luacmartins Release conflicts, please!

luacmartins · 2022-11-04T18:44:16Z

@Santhosh-Sellavel updated!

mountiny · 2022-11-04T19:07:32Z

LGTM! We should get someone internal to approve too

Santhosh-Sellavel · 2022-11-04T19:26:48Z

Testing this now!

Santhosh-Sellavel · 2022-11-04T20:00:03Z

Screen Recordings

Unable to add android recording due to this issue

iOS Only online is added, having a problem with simulating offline!

#### WEB & DESKTOP ##### OFFLINE/ONLINE CANCEL https://user-images.githubusercontent.com/85645967/200063620-7b2b6578-4b6f-43d7-b843-0c5300627573.mov

OFFLINE/ONLINE DECLINE

DESKTOP_WEB_OFFLINE_ONLINE_DECLINE.mov

ONLINE DECLINE & CANCEL

DESKTOP_WEB_ONLINE.mov

IOS & ANDROID MWEB

OFFLINE_ONLINE_CANCEL

ANDROID_MWEB_CANCEL_OFFLINE_ONLINE.mov

OFFLINE_ONLINE_DECLINE

IOS_MWEB_DECLINE_OFFLINE_ONLINE.mov

ONLINE

Screen.Recording.2022-11-05.at.2.01.49.AM.mov

iOS

Online

ONLINE.mp4

Santhosh-Sellavel · 2022-11-04T21:18:13Z

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Santhosh-Sellavel

Checklist: #11720 (comment)
Recordings: #11720 (comment)

Only one unanswered question here. What about toggling balance?
As the above issue might be unrelated to this as it could occur during the creation flow also, so approving this one!

Looks good!
C+ Reviewed
🎀 👀 🎀

luacmartins · 2022-11-04T22:00:55Z

@Santhosh-Sellavel I address that comment here #11720 (comment). This is unrelated to this PR and I'll create another issue for this.

luacmartins · 2022-11-04T22:01:06Z

Merging this PR since we have 3 approvals!

luacmartins · 2022-11-04T22:01:16Z

Thanks everyone for the reviews and tests!

melvin-bot · 2022-11-04T22:01:29Z

@luacmartins looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

luacmartins · 2022-11-04T22:02:53Z

Tests passed. Removing Emergency label.

OSBotify · 2022-11-04T22:06:29Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-11-08T19:39:09Z

🚀 Deployed to staging by @luacmartins in version: 1.2.25-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-11-09T21:14:11Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.25-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

aldo-expensify · 2023-04-28T14:57:49Z

src/libs/actions/IOU.js

+    chatReport.maxSequenceNumber = newSequenceNumber;
+    chatReport.lastReadSequenceNumber = newSequenceNumber;
+    chatReport.lastMessageText = optimisticReportAction.message[0].text;
+    chatReport.lastMessageHtml = optimisticReportAction.message[0].html;
+    chatReport.hasOutstandingIOU = updatedIOUReport.total !== 0;


This is mutating an object that is stored in Onyx, This object is very likely to be used later as props of react components. Mutations make the === unreliable for checking if an object changed or not. I don't think we should be merging code with this sort of mutations.

mountiny self-assigned this Oct 11, 2022

Refactoring the reject transaction to cancel money request

ff85464

mountiny force-pushed the vit-rejectTransaction branch from d1a1ec4 to ff85464 Compare October 11, 2022 21:55

mountiny added 2 commits October 12, 2022 14:57

Remove deprecated export

bc79770

Merge branch 'main' into vit-rejectTransaction

cc66f3b

mountiny changed the title ~~[WIP] Refactor RejectTransaction to CancelIOUTransaction~~ Refactor RejectTransaction to CancelIOUTransaction Oct 13, 2022

mountiny added 9 commits October 13, 2022 19:44

Move transaction to ReportTransaction component

f884152

Fix report id types

51e910f

clean up

2200d96

Use switch and add offlineWithFeedback component to IOUTransaction

c3840b2

Lint errors

516dae7

Merge branch 'main' into vit-rejectTransaction

c6c7745

Make the optimisti total updates working and update propTypes

0db62f1

Merge branch 'main' into vit-rejectTransaction

a0eea41

Improve comments and remove console log

0d0d441

mountiny commented Oct 21, 2022

View reviewed changes

src/libs/actions/IOU.js Outdated Show resolved Hide resolved

luacmartins self-assigned this Oct 31, 2022

luacmartins added 12 commits October 31, 2022 13:10

resolve conflicts

2adffd3

rm log, use sequenceNumber, update comments

23c3f80

use updateIOUOwnerAndTotal

ad80669

rollback iouReport on failure

8859584

use errors key

9d924c3

use type

d13d32c

resolve conflicts

38b7ccf

add other types to getIOUReportActionMessage

3282eb2

fix args for buildOptimisticIOUReportAction

a5f8583

pass correct transactionID in request money

15ddce0

add additional params

741726b

;

fix iou report key

36f1ee0

Gonals self-requested a review November 4, 2022 11:16

Gonals previously approved these changes Nov 4, 2022

View reviewed changes

src/components/ReportTransaction.js Outdated Show resolved Hide resolved

src/libs/actions/IOU.js Show resolved Hide resolved

resolve conflicts

ad3b7c3

luacmartins dismissed Gonals’s stale review via ad3b7c3 November 4, 2022 18:42

use const

afb8e63

Gonals approved these changes Nov 4, 2022

View reviewed changes

Luke9389 approved these changes Nov 4, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Nov 4, 2022

View reviewed changes

luacmartins merged commit 5021ae4 into main Nov 4, 2022

luacmartins deleted the vit-rejectTransaction branch November 4, 2022 22:01

melvin-bot bot added the Emergency label Nov 4, 2022

luacmartins removed the Emergency label Nov 4, 2022

luacmartins mentioned this pull request Nov 7, 2022

Amount splits two times if I keep pressing Split button - reported by @adeel0202 #8722

Closed

OSBotify mentioned this pull request Nov 8, 2022

Deploy Checklist: New Expensify 2022-11-08 #12563

Closed

34 tasks

luacmartins mentioned this pull request Nov 8, 2022

IOU - No loading state when click cancel in the money request details page and page closes abruptly #12569

Closed

aldo-expensify reviewed Apr 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor RejectTransaction to CancelIOUTransaction #11720

Refactor RejectTransaction to CancelIOUTransaction #11720

mountiny commented Oct 11, 2022 •

edited by luacmartins

Loading

melvin-bot bot commented Oct 11, 2022

Santhosh-Sellavel commented Nov 4, 2022

luacmartins commented Nov 4, 2022

mountiny commented Nov 4, 2022

Santhosh-Sellavel commented Nov 4, 2022

Santhosh-Sellavel commented Nov 4, 2022 •

edited

Loading

Screen Recordings

OFFLINE/ONLINE DECLINE

ONLINE DECLINE & CANCEL

IOS & ANDROID MWEB

OFFLINE_ONLINE_CANCEL

OFFLINE_ONLINE_DECLINE

ONLINE

iOS

Online

Santhosh-Sellavel commented Nov 4, 2022 •

edited

Loading

PR Reviewer Checklist

Santhosh-Sellavel left a comment

luacmartins commented Nov 4, 2022

luacmartins commented Nov 4, 2022

luacmartins commented Nov 4, 2022

melvin-bot bot commented Nov 4, 2022

luacmartins commented Nov 4, 2022

OSBotify commented Nov 4, 2022

OSBotify commented Nov 8, 2022

OSBotify commented Nov 9, 2022

aldo-expensify Apr 28, 2023

Refactor RejectTransaction to CancelIOUTransaction #11720

Refactor RejectTransaction to CancelIOUTransaction #11720

Conversation

mountiny commented Oct 11, 2022 • edited by luacmartins Loading

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot bot commented Oct 11, 2022

Santhosh-Sellavel commented Nov 4, 2022

luacmartins commented Nov 4, 2022

mountiny commented Nov 4, 2022

Santhosh-Sellavel commented Nov 4, 2022

Santhosh-Sellavel commented Nov 4, 2022 • edited Loading

Screen Recordings

OFFLINE/ONLINE DECLINE

ONLINE DECLINE & CANCEL

IOS & ANDROID MWEB

OFFLINE_ONLINE_CANCEL

OFFLINE_ONLINE_DECLINE

ONLINE

iOS

Online

Santhosh-Sellavel commented Nov 4, 2022 • edited Loading

PR Reviewer Checklist

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

luacmartins commented Nov 4, 2022

luacmartins commented Nov 4, 2022

luacmartins commented Nov 4, 2022

melvin-bot bot commented Nov 4, 2022

luacmartins commented Nov 4, 2022

OSBotify commented Nov 4, 2022

OSBotify commented Nov 8, 2022

OSBotify commented Nov 9, 2022

aldo-expensify Apr 28, 2023

Choose a reason for hiding this comment

mountiny commented Oct 11, 2022 •

edited by luacmartins

Loading

Santhosh-Sellavel commented Nov 4, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 4, 2022 •

edited

Loading